Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XWIKI-22260: WCAG automatic testing for modals #3293

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Sereza7
Copy link
Contributor

@Sereza7 Sereza7 commented Jul 22, 2024

Jira URL

https://jira.xwiki.org/browse/XWIKI-22260

Changes

Description

  • Added wcag validation to modal elements.
  • Fixed a code style inconsistency in the BasePage wcag validation

Clarifications

  • I did not run a full build with WCAG testing yet to check if there's any tests failures because of these new checks. So there's two solutions to merge this:
    • Consider there's not that many test suites with modal elements, merge this PR as is, and take care of the state of continuous integration for the docker tests. If fails are found on CI, provide a fix asap (either a real fix or swap the type of the rule from fail to warning until a proper solution is found).
    • Add a safeguard in the WCAG testing to allow tests that never fail. This has the advantage of being reusable if at some point we want to add automated tests on other elements that can be found in multiple places at the level of the project. However as things stand it might not be needed, and on the long term this feature is useless.
  • The new code is almost a duplicate of the one in the basePage object. I was not sure how or where I could factorize this. If deemed necessary, I'll factorize this. I could use some help finding out where to keep this function. At first, I thought of WCAGUtils, but it doesn't share much scope with the page objects, so I scrapped it. Then I thought of BaseElement
    , which is the parent of all the elements we would want. This looked pretty good, but I wasn't sure of the impact on performance adding a quite large method and dependencies to all the objects would have (I got a look at the class, it's pretty empty as of now, I'm not sure adding something specific to WCAG testing was correct). At last, I thought of having a new interface for WCAG analyzable objects, IMO this is the best alternative, but I don't know in what module I'd include it in... Any of your opinion or help is welcome. I'm certain duplicating code like done now is not good for maintanability and I'd want to avoid it.
  • Some of the modal objects do not extend BaseModal e.g. MacroDialogEditModal . Those will not be tested yet, and should be fixed later on so that they can get their own tests. Some do not call their super() constructor, same effect, they will not be tested.

Screenshots & Video

None, test changes only.

Executed Tests

I ran an emptied out version of flamingo skin docker tests, which only had the XAR export IT (few tests with instantiations of modals) with the command:

mvn clean install -f xwiki-platform-core/xwiki-platform-test/xwiki-platform-test-ui; mvn clean install -f xwiki-platform-core/xwiki-platform-flamingo/xwiki-platform-flamingo-skin/xwiki-platform-flamingo-skin-test/xwiki-platform-flamingo-skin-test-docker -Dxwiki.test.ui.wcag=true;spd-say "DONE"

Without the changes in this PR, I had only 306 WCAG rule checks. With the changes in the PR, I have 361. When looking at the logs with the right parameters, I could see that as expected, only two modals got checked. All the other ones did not get checked because already in the cache. 55 rules checked seems like the order of magnitude I'd expect for two checks on a part of a page.

Out of the 55 new rule checks, 2 were marked as incomplete (automated testing alone couldn't deliver a proper answer) and all the others were passed succesfully. These numbers are in line with the repartition of current rule check results. Importantly, no rule check was failed.

I did not test everything to make sure none of the new checks failed. See the first item in the clarifications section above for more info about the strategies we could go with onwards.

Expected merging strategy

  • Prefers squash: Yes
  • Backport on branches:
    • None, the changes are pretty dangerous for the CI and not needed on more than one branch IMO. Might as well keep it on master.

* Added wcag validation to modal elements.
* Fixed a code style inconsistency in the BasePage wcag validation
if (wcagContext.shouldWCAGStopOnError()) {
throw e;
} else {
LOGGER.debug("Error during WCAG execution, but ignored thanks to wcagStopOnError flag: ", e);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better be an info than a debug here no? We don't enable debug often, and it could be useful to still see those, or could that be an issue on the CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is in line with

LOGGER.debug("Error during WCAG execution, but ignored thanks to wcagStopOnError flag: ", e);

@@ -51,6 +59,9 @@ public BaseModal(By selector)
String className = this.container.getAttribute("class");
className = className.replace("fade", "");
getDriver().executeScript("arguments[0].setAttribute(\"class\",arguments[1])", this.container, className);
// Once we're sure the modal is loaded, we can validate its content in regards to accessibility
className = this.container.getAttribute("class").split(" ")[0];
validateWCAG(getUtil().getWCAGUtils().getWCAGContext(), true, "." + className);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels dangerous to rely on the className only: what if you have a page with 3 modals having the class modal, and only one is loaded? Right now the validation will be executed on the 3 modals AFAIU.
Feels like you could artificially add a new specific class (pretty much like we remove the fade class) just to identify the actual modal.

@Sereza7 Sereza7 marked this pull request as draft August 26, 2024 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants